Skip to content

Conversation

@yaya1738
Copy link

@yaya1738 yaya1738 commented Dec 4, 2025

Summary

Implements package import functionality as requested in #126:

  • requirements.txt (Python/pip) - with version specifiers, extras, markers
  • package.json (Node.js/npm) - dependencies and devDependencies
  • Gemfile (Ruby/bundler) - with group support
  • Cargo.toml (Rust/cargo) - simple TOML parser for dependencies
  • go.mod (Go modules) - require blocks, skips indirect

Features

  • Auto-detection of requirements files in project directory
  • Unified CLI interface: cortex import <file> or cortex import --all
  • Dry-run mode for preview: cortex import --dry-run
  • Dev dependency support: cortex import --dev
  • Verbose output mode
  • Comprehensive error handling

Usage

# Import from a single file
cortex import requirements.txt
cortex import package.json

# Import from all detected files
cortex import --all

# Preview what would be installed
cortex import --dry-run requirements.txt

# Include dev dependencies
cortex import --dev package.json

# Detect files without importing
cortex import --detect

Test Plan

  • 50+ unit tests covering all parsers
  • Tests for edge cases (invalid JSON, missing files, etc.)
  • Integration tests with realistic file structures
  • All tests passing locally

Files Added

  • cortex/requirements_importer.py - Core implementation (~650 lines)
  • cortex/import_cli.py - CLI commands (~190 lines)
  • tests/test_requirements_importer.py - Unit tests (~760 lines, 50 tests)
  • README_IMPORT.md - Documentation

Closes #126

Summary by CodeRabbit

Release Notes

  • New Features

    • Added import command to automatically detect and parse dependency files from Python, Node.js, Ruby, Rust, and Go projects
    • Supports dry-run mode to preview changes and verbose output for detailed feedback
  • Documentation

    • Added comprehensive documentation for the import feature with usage examples and command reference

✏️ Tip: You can customize this high-level summary in your review settings.

…ortexlinux#126)

Implements package import functionality for:
- requirements.txt (Python/pip)
- package.json (Node.js/npm)
- Gemfile (Ruby/bundler)
- Cargo.toml (Rust/cargo)
- go.mod (Go modules)

Features:
- Auto-detection of requirements files
- Dry-run mode for preview
- Dev dependency support
- Unified CLI interface
- 50+ unit tests
Copilot AI review requested due to automatic review settings December 4, 2025 07:14
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 4, 2025

Walkthrough

This PR introduces a unified multi-language dependency import and installation system for Cortex. It adds support for parsing Python requirements.txt, Node.js package.json, Ruby Gemfile, Rust Cargo.toml, and Go go.mod files, with corresponding CLI commands for detection, parsing, and package manager installation across all ecosystems.

Changes

Cohort / File(s) Summary
Documentation
README_IMPORT.md
Added comprehensive documentation for Cortex Import feature, covering quick start, supported formats, command variants, options, example formats for all five languages, Python API usage, exit codes, and file mappings.
CLI Interface
cortex/import_cli.py
Introduced import CLI module with create_import_parser() to build subcommand, handle_import() to orchestrate workflow with detect/all/single-file modes and dry-run support, and main() entry point for standalone usage.
Core Implementation
cortex/requirements_importer.py
Added RequirementsImporter orchestrator, multi-language parsers (RequirementsParser, PackageJsonParser, GemfileParser, CargoTomlParser, GoModParser), data models (Dependency, ParseResult, PackageManager enum), and language-specific installers for pip, npm, bundler, cargo, and go.
Test Suite
tests/test_requirements_importer.py
Added comprehensive test coverage for all parsers, data models, orchestrator logic, installation workflows, dry-run modes, error handling, and integration scenarios across all five ecosystems.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant CLI as import_cli
    participant Importer as RequirementsImporter
    participant Parsers as Language Parsers
    participant PackageManager as Package Manager<br/>(pip/npm/etc)

    User->>CLI: cortex import file.txt [--all|--detect|--dry-run]
    
    alt detect mode
        CLI->>Importer: detect_files(dir)
        Importer->>Importer: scan directory
        Importer-->>CLI: list of requirement files
        CLI-->>User: display detected files
    else all mode
        CLI->>Importer: parse_all(dir)
        loop for each detected file
            Importer->>Parsers: dispatch to language parser
            Parsers->>Parsers: parse dependencies
            Parsers-->>Importer: ParseResult
            Importer->>Importer: accumulate results
        end
        Importer-->>CLI: all ParseResults
        CLI-->>User: display per-file summary
        alt dry-run
            CLI-->>User: show what would install
        else normal install
            loop for each ParseResult
                CLI->>Importer: install(result)
                Importer->>PackageManager: run pip/npm/bundler/cargo/go
                PackageManager-->>Importer: success/failure
                Importer-->>CLI: result
            end
            CLI-->>User: installation complete
        end
    else single file mode
        CLI->>Importer: parse_file(filepath)
        Importer->>Parsers: dispatch to appropriate parser
        Parsers->>Parsers: parse dependencies
        Parsers-->>Importer: ParseResult
        Importer-->>CLI: ParseResult
        CLI-->>User: display dependencies
        alt dry-run
            CLI-->>User: show what would install
        else normal install
            CLI->>Importer: install(result)
            Importer->>PackageManager: run appropriate installer
            PackageManager-->>Importer: success/failure
            Importer-->>CLI: result
            CLI-->>User: installation complete
        end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Areas requiring extra attention:

  • cortex/requirements_importer.py: The five distinct language parsers (RequirementsParser, PackageJsonParser, GemfileParser, CargoTomlParser, GoModParser) each implement different parsing logic with varying complexity; validate correctness of regex/parsing logic for each format and error handling paths.
  • Package manager integration: The _install_* methods delegate to subprocess calls for pip, npm, bundler, cargo, and go; verify subprocess invocation correctness, exit code handling, and dev dependency flag threading.
  • CLI workflow orchestration: handle_import() implements multiple modes (detect, all, single-file, dry-run) with conditional logic; ensure all paths are tested and user-facing messages are consistent.
  • Data model usage: Dependency and ParseResult dataclasses are used across all parsers; confirm consistency in how extras, markers, dev dependencies, and errors are propagated through the system.

Suggested reviewers

  • mikejmorgan-ai

Poem

🐰 Five formats, one heart, a unified way,
Parse and install—no more sorting the fray!
From Python to Rust, each ecosystem we've caught,
Dependencies bundled with code-loving thought.

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding a requirements importer for multi-language dependency files, directly referencing issue #126.
Description check ✅ Passed The description covers all template requirements: summary of changes, type of change (new feature), testing confirmation, and links to closing issue. It provides comprehensive details on supported formats, features, usage, and test coverage.
Linked Issues check ✅ Passed The PR fulfills all coding requirements from issue #126: parsers for Python, Node.js, Ruby, Rust, and Go; unified CLI interface; dry-run and dev dependency support; comprehensive test suite (50+ tests); and documentation.
Out of Scope Changes check ✅ Passed All changes are in scope: requirements_importer.py and import_cli.py implement the multi-language parsing and installation feature; tests validate the implementation; README_IMPORT.md documents the feature as required.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 4, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
1 Security Hotspot
B Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Nitpick comments (5)
cortex/import_cli.py (2)

80-178: Consider refactoring to reduce cognitive complexity.

SonarCloud flags this function with cognitive complexity of 53 (limit: 15). While the logic is correct, extracting helper functions would improve maintainability.

Consider extracting the three modes into separate functions:

def _handle_detect_mode(importer: RequirementsImporter, directory: str) -> int:
    """Handle --detect mode."""
    files = importer.detect_files(directory)
    if files:
        print("Detected requirements files:")
        for f in files:
            print(f"  - {f}")
    else:
        print("No requirements files detected.")
    return 0


def _handle_import_all_mode(importer: RequirementsImporter, args: argparse.Namespace) -> int:
    """Handle --all mode."""
    # ... existing import-all logic ...


def _handle_single_file_mode(importer: RequirementsImporter, args: argparse.Namespace) -> int:
    """Handle single file import."""
    # ... existing single-file logic ...

Then handle_import becomes a simple dispatcher.


181-213: DRY violation: argument definitions are duplicated.

The arguments defined in main() (lines 193-204) duplicate those in create_import_parser() (lines 31-73). If arguments change, both locations must be updated.

Consider extracting common argument configuration:

def _add_import_arguments(parser: argparse.ArgumentParser) -> None:
    """Add import-related arguments to a parser."""
    parser.add_argument('file', nargs='?', help='Requirements file to import')
    parser.add_argument('--all', '-a', action='store_true', dest='import_all', ...)
    # ... other arguments ...
    parser.set_defaults(func=handle_import)


def create_import_parser(subparsers):
    import_parser = subparsers.add_parser('import', ...)
    _add_import_arguments(import_parser)
    return import_parser


def main(args=None):
    parser = argparse.ArgumentParser(prog='cortex-import', ...)
    _add_import_arguments(parser)
    # ... rest of main ...
cortex/requirements_importer.py (3)

287-338: Consider using a TOML parser library for robustness.

The hand-rolled TOML parsing may miss edge cases like:

  • Multi-line strings
  • Inline tables with nested brackets
  • Comments within dependency declarations

Consider using tomllib (Python 3.11+) or tomli for proper TOML parsing:

try:
    import tomllib
except ImportError:
    import tomli as tomllib

@staticmethod
def parse(file_path: str) -> ParseResult:
    # ...
    with open(path, 'rb') as f:
        data = tomllib.load(f)
    
    for name, value in data.get('dependencies', {}).items():
        version = value if isinstance(value, str) else value.get('version')
        # ...

The current implementation works for common cases but may fail on complex Cargo.toml files.


480-520: Refactor parse_file to reduce cognitive complexity.

SonarCloud flags complexity of 17 (limit 15). The parser selection logic has redundant paths.

The current logic has overlapping conditions. Consider simplifying:

def parse_file(self, file_path: str) -> ParseResult:
    """Parse a single requirements file."""
    filename = Path(file_path).name
    
    # Direct filename match
    if filename in self.PARSERS:
        parser = self.PARSERS[filename]
    # Requirements variants (requirements-*.txt, etc.)
    elif filename.startswith('requirements') and filename.endswith('.txt'):
        parser = RequirementsParser
    # Extension-based fallback
    else:
        parser = self._get_parser_by_extension(file_path)
    
    if parser is None:
        return ParseResult(
            dependencies=[], dev_dependencies=[],
            package_manager=PackageManager.PIP,
            source_file=file_path,
            errors=[f"Unknown file type: {file_path}"]
        )
    
    result = parser.parse(file_path)
    self.results.append(result)
    return result

def _get_parser_by_extension(self, file_path: str) -> Optional[type]:
    """Get parser based on file extension."""
    ext_map = {
        '.txt': RequirementsParser,
        '.json': PackageJsonParser,
        '.toml': CargoTomlParser,
        '.mod': GoModParser,
    }
    for ext, parser in ext_map.items():
        if file_path.endswith(ext):
            return parser
    if 'Gemfile' in file_path:
        return GemfileParser
    return None

558-559: Avoid catching blind Exception.

Catching Exception can mask unexpected errors. Consider catching specific exceptions.

-        except Exception as e:
+        except (subprocess.SubprocessError, OSError, ValueError) as e:
             return False, str(e)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between da3e635 and bfac19c.

📒 Files selected for processing (4)
  • README_IMPORT.md (1 hunks)
  • cortex/import_cli.py (1 hunks)
  • cortex/requirements_importer.py (1 hunks)
  • tests/test_requirements_importer.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
cortex/import_cli.py (2)
cortex/requirements_importer.py (4)
  • PackageManager (23-29)
  • detect_files (468-478)
  • parse_file (480-520)
  • install (533-559)
src/sandbox_executor.py (1)
  • success (52-54)
tests/test_requirements_importer.py (1)
cortex/requirements_importer.py (19)
  • CargoTomlParser (287-376)
  • Dependency (33-44)
  • GemfileParser (221-284)
  • GoModParser (379-446)
  • PackageJsonParser (153-218)
  • PackageManager (23-29)
  • ParseResult (48-54)
  • RequirementsImporter (449-648)
  • RequirementsParser (57-150)
  • parse (61-103)
  • parse (157-211)
  • parse (225-284)
  • parse (291-338)
  • parse (383-446)
  • detect_files (468-478)
  • parse_file (480-520)
  • parse_all (522-531)
  • install (533-559)
  • summary (635-648)
🪛 GitHub Check: SonarCloud Code Analysis
cortex/import_cli.py

[failure] 80-80: Refactor this function to reduce its Cognitive Complexity from 53 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=cortexlinux_cortex&issues=AZroNwOhrqkqI7vQN9gl&open=AZroNwOhrqkqI7vQN9gl&pullRequest=238


[warning] 112-112: Remove the unused local variable "total_deps".

See more on https://sonarcloud.io/project/issues?id=cortexlinux_cortex&issues=AZroNwOhrqkqI7vQN9gk&open=AZroNwOhrqkqI7vQN9gk&pullRequest=238

tests/test_requirements_importer.py

[warning] 590-590: Replace the unused local variable "message" with "_".

See more on https://sonarcloud.io/project/issues?id=cortexlinux_cortex&issues=AZroNwRTrqkqI7vQN9gq&open=AZroNwRTrqkqI7vQN9gq&pullRequest=238


[warning] 610-610: Replace the unused local variable "message" with "_".

See more on https://sonarcloud.io/project/issues?id=cortexlinux_cortex&issues=AZroNwRTrqkqI7vQN9gr&open=AZroNwRTrqkqI7vQN9gr&pullRequest=238

cortex/requirements_importer.py

[failure] 480-480: Refactor this function to reduce its Cognitive Complexity from 17 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=cortexlinux_cortex&issues=AZroNwRHrqkqI7vQN9gp&open=AZroNwRHrqkqI7vQN9gp&pullRequest=238


[failure] 383-383: Refactor this function to reduce its Cognitive Complexity from 20 to the 15 allowed.

See more on https://sonarcloud.io/project/issues?id=cortexlinux_cortex&issues=AZroNwRHrqkqI7vQN9go&open=AZroNwRHrqkqI7vQN9go&pullRequest=238


[warning] 129-129: Rework this part of the regex to not match the empty string.

See more on https://sonarcloud.io/project/issues?id=cortexlinux_cortex&issues=AZroNwRHrqkqI7vQN9gn&open=AZroNwRHrqkqI7vQN9gn&pullRequest=238

🪛 Ruff (0.14.7)
tests/test_requirements_importer.py

590-590: Unpacked variable message is never used

Prefix it with an underscore or any other dummy variable pattern

(RUF059)


610-610: Unpacked variable message is never used

Prefix it with an underscore or any other dummy variable pattern

(RUF059)


714-714: Unused method argument: capsys

(ARG002)

cortex/requirements_importer.py

131-131: Avoid specifying long messages outside the exception class

(TRY003)


243-243: Loop control variable line_num not used within loop body

Rename unused line_num to _line_num

(B007)


329-329: Do not catch blind exception: Exception

(BLE001)


401-401: Loop control variable line_num not used within loop body

Rename unused line_num to _line_num

(B007)


453-461: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)


558-558: Do not catch blind exception: Exception

(BLE001)


572-572: Consider iterable unpacking instead of concatenation

Replace with iterable unpacking

(RUF005)


576-576: subprocess call: check for execution of untrusted input

(S603)


585-585: Consider ['npm', 'install', *packages] instead of concatenation

Replace with ['npm', 'install', *packages]

(RUF005)


589-589: subprocess call: check for execution of untrusted input

(S603)


603-603: subprocess call: check for execution of untrusted input

(S603)


617-617: subprocess call: check for execution of untrusted input

(S603)


630-630: subprocess call: check for execution of untrusted input

(S603)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Analyze (python)
  • GitHub Check: Agent
🔇 Additional comments (10)
README_IMPORT.md (1)

1-235: Documentation is comprehensive and well-organized.

The README covers all key aspects: CLI usage, supported formats, options, file format examples, Python API, and exit codes. The quick start section provides immediate value for users.

Minor suggestion: Consider adding a troubleshooting section for common issues (e.g., package manager not installed, permission errors).

cortex/import_cli.py (1)

22-77: LGTM!

The argument parser is well-structured with appropriate short flags and help text. The use of dest='import_all' to avoid conflict with Python's all builtin is a good practice.

tests/test_requirements_importer.py (5)

26-43: LGTM!

Tests cover the essential Dependency dataclass behaviors including string representation with and without version, extras, and markers.


46-157: Good test coverage for Python requirements parsing.

The tests cover key scenarios: simple requirements, versions, comments, extras, markers, empty lines, file not found, and recursive includes. The cleanup pattern is functional.


160-310: Good coverage for package.json and Gemfile parsers.

Tests cover dependencies, devDependencies, groups, comments, empty files, invalid JSON, and file not found scenarios.


312-488: Good coverage for Cargo.toml and go.mod parsers.

Tests cover simple dependencies, dev-dependencies, complex dependencies, require blocks, indirect dependencies, and edge cases.


728-762: Integration test provides good end-to-end validation.

The test creates a realistic project structure with multiple dependency files and validates the full workflow: detection, parsing, and dry-run installation.

cortex/requirements_importer.py (3)

32-55: LGTM!

Dataclasses are well-designed with proper use of field(default_factory=list) for mutable default values.


57-150: Requirements parser implementation is solid.

Good handling of:

  • Version specifiers (==, >=, <=, >, <, ~=, !=)
  • Extras (package[extra1,extra2])
  • Environment markers
  • Comments (line and inline)
  • Recursive includes (with warning)

The regex on line 129 matching an empty remainder is intentional for packages without version specifiers.


561-592: Security consideration: subprocess calls with user-controlled input.

Package names parsed from requirement files are passed directly to subprocess commands. A malicious package name could potentially exploit shell behavior.

While subprocess.run with a list of arguments (not shell=True) provides some protection, verify that:

  1. Package managers (pip, npm) properly validate package names
  2. No shell expansion occurs with special characters

The current implementation uses lists (not shell=True), which is good. However, consider adding basic validation of package names:

import re

def _validate_package_name(name: str) -> bool:
    """Basic validation to reject obviously malicious package names."""
    # Reject names with shell metacharacters
    if re.search(r'[;&|`$<>]', name):
        return False
    return True

Also, prefer unpacking as suggested by static analysis:

-        cmd = [sys.executable, '-m', 'pip', 'install'] + packages
+        cmd = [sys.executable, '-m', 'pip', 'install', *packages]
-        cmd = ['npm', 'install'] + packages
+        cmd = ['npm', 'install', *packages]

Comment on lines +112 to +114
total_deps = len(result.dependencies)
if args.dev:
total_deps += len(result.dev_dependencies)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove unused variable total_deps.

The variable is computed but never used. As flagged by static analysis.

-            total_deps = len(result.dependencies)
-            if args.dev:
-                total_deps += len(result.dev_dependencies)
-
             print(f"\n{file_path} ({result.package_manager.value}):")
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis

[warning] 112-112: Remove the unused local variable "total_deps".

See more on https://sonarcloud.io/project/issues?id=cortexlinux_cortex&issues=AZroNwOhrqkqI7vQN9gk&open=AZroNwOhrqkqI7vQN9gk&pullRequest=238

🤖 Prompt for AI Agents
In cortex/import_cli.py around lines 112 to 114, the variable total_deps is
computed but never used; remove the unused variable and its related increment
(delete the two lines that assign and modify total_deps) or, if a dependency
count is intended, replace its usage by actually using total_deps where needed
(e.g., in progress/output). Ensure no remaining references to total_deps remain
to avoid NameError.

)

with open(path, 'r') as f:
for line_num, line in enumerate(f, 1):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Rename unused loop variable.

The line_num variable is not used in the loop body.

-            for line_num, line in enumerate(f, 1):
+            for _line_num, line in enumerate(f, 1):

Or simply:

-            for line_num, line in enumerate(f, 1):
+            for line in f:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for line_num, line in enumerate(f, 1):
for _line_num, line in enumerate(f, 1):
Suggested change
for line_num, line in enumerate(f, 1):
for line in f:
🧰 Tools
🪛 Ruff (0.14.7)

243-243: Loop control variable line_num not used within loop body

Rename unused line_num to _line_num

(B007)

🤖 Prompt for AI Agents
In cortex/requirements_importer.py around line 243, the for-loop declares an
unused variable named line_num; rename it to _ (or _line_num) or remove it
entirely so the unused variable warning is resolved; update any references if
later used, otherwise iterate as for _, line in enumerate(f, 1): or simply for
line in f: to keep semantics identical.

in_require_block = False

with open(path, 'r') as f:
for line_num, line in enumerate(f, 1):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Rename unused loop variable.

Same issue as GemfileParser - line_num is not used.

-            for line_num, line in enumerate(f, 1):
+            for line in f:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for line_num, line in enumerate(f, 1):
for line in f:
🧰 Tools
🪛 Ruff (0.14.7)

401-401: Loop control variable line_num not used within loop body

Rename unused line_num to _line_num

(B007)

🤖 Prompt for AI Agents
In cortex/requirements_importer.py around line 401, the enumerate loop uses an
unused variable named line_num; rename it to _line_num or simply _ to avoid
unused-variable lint warnings and clarify intent (e.g., for line enumeration
that doesn’t use the index, change "for line_num, line in enumerate(f, 1):" to
"for _, line in enumerate(f, 1):" or "for _line_num, line in enumerate(f, 1):").
Ensure no other references to line_num remain.

Comment on lines +453 to +461
PARSERS = {
'requirements.txt': RequirementsParser,
'requirements-dev.txt': RequirementsParser,
'requirements_dev.txt': RequirementsParser,
'package.json': PackageJsonParser,
'Gemfile': GemfileParser,
'Cargo.toml': CargoTomlParser,
'go.mod': GoModParser,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Annotate class variable with ClassVar.

PARSERS is a mutable class attribute and should be annotated with typing.ClassVar to indicate it's not an instance attribute.

+from typing import ClassVar, Dict, List, Optional, Tuple, Type

 class RequirementsImporter:
     """Main class for importing and installing dependencies."""

     # Mapping of file names to parsers
-    PARSERS = {
+    PARSERS: ClassVar[Dict[str, Type]] = {
         'requirements.txt': RequirementsParser,
🧰 Tools
🪛 Ruff (0.14.7)

453-461: Mutable class attributes should be annotated with typing.ClassVar

(RUF012)

🤖 Prompt for AI Agents
In cortex/requirements_importer.py around lines 453 to 461, the PARSERS mutable
class attribute should be annotated as a ClassVar; update the declaration to
something like "PARSERS: ClassVar[Dict[str, Type[BaseParser]]]" (replace
BaseParser with the actual parser base class name used in the module) and add
"from typing import ClassVar, Dict, Type" to the imports if not present; leave
the dictionary value exactly the same but ensure the type annotation marks it as
a class-level variable.

Comment on lines +580 to +598
@patch('cortex.requirements_importer.subprocess.run')
def test_install_pip(self, mock_run):
mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="")

with tempfile.NamedTemporaryFile(mode='w', suffix='.txt', delete=False) as f:
f.write("flask==2.0.1\nrequests\n")
f.flush()

importer = RequirementsImporter()
result = importer.parse_file(f.name)
success, message = importer.install(result)

os.unlink(f.name)

assert success
mock_run.assert_called_once()
call_args = mock_run.call_args[0][0]
assert 'pip' in ' '.join(call_args)
assert 'install' in call_args
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Prefix unused variable with underscore.

The message variable is unpacked but never used. Prefix with underscore to indicate intentional discard.

-            success, message = importer.install(result)
+            success, _message = importer.install(result)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@patch('cortex.requirements_importer.subprocess.run')
def test_install_pip(self, mock_run):
mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="")
with tempfile.NamedTemporaryFile(mode='w', suffix='.txt', delete=False) as f:
f.write("flask==2.0.1\nrequests\n")
f.flush()
importer = RequirementsImporter()
result = importer.parse_file(f.name)
success, message = importer.install(result)
os.unlink(f.name)
assert success
mock_run.assert_called_once()
call_args = mock_run.call_args[0][0]
assert 'pip' in ' '.join(call_args)
assert 'install' in call_args
@patch('cortex.requirements_importer.subprocess.run')
def test_install_pip(self, mock_run):
mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="")
with tempfile.NamedTemporaryFile(mode='w', suffix='.txt', delete=False) as f:
f.write("flask==2.0.1\nrequests\n")
f.flush()
importer = RequirementsImporter()
result = importer.parse_file(f.name)
success, _message = importer.install(result)
os.unlink(f.name)
assert success
mock_run.assert_called_once()
call_args = mock_run.call_args[0][0]
assert 'pip' in ' '.join(call_args)
assert 'install' in call_args
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis

[warning] 590-590: Replace the unused local variable "message" with "_".

See more on https://sonarcloud.io/project/issues?id=cortexlinux_cortex&issues=AZroNwRTrqkqI7vQN9gq&open=AZroNwRTrqkqI7vQN9gq&pullRequest=238

🪛 Ruff (0.14.7)

590-590: Unpacked variable message is never used

Prefix it with an underscore or any other dummy variable pattern

(RUF059)

🤖 Prompt for AI Agents
In tests/test_requirements_importer.py around lines 580 to 598, the test unpacks
the return of importer.install(result) into success, message but never uses
message; change the unpack to success, _message (or success, _ ) to indicate the
variable is intentionally unused and update the variable name in that assignment
only.

Comment on lines +600 to +615
@patch('cortex.requirements_importer.subprocess.run')
def test_install_npm(self, mock_run):
mock_run.return_value = MagicMock(returncode=0, stdout="", stderr="")

with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as f:
json.dump({"dependencies": {"express": "^4.0.0"}}, f)
f.flush()

importer = RequirementsImporter()
result = importer.parse_file(f.name)
success, message = importer.install(result)

os.unlink(f.name)

assert success
mock_run.assert_called_once()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Prefix unused variable with underscore.

Same issue as above - message is unpacked but not used.

-            success, message = importer.install(result)
+            success, _message = importer.install(result)
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis

[warning] 610-610: Replace the unused local variable "message" with "_".

See more on https://sonarcloud.io/project/issues?id=cortexlinux_cortex&issues=AZroNwRTrqkqI7vQN9gr&open=AZroNwRTrqkqI7vQN9gr&pullRequest=238

🪛 Ruff (0.14.7)

610-610: Unpacked variable message is never used

Prefix it with an underscore or any other dummy variable pattern

(RUF059)

🤖 Prompt for AI Agents
In tests/test_requirements_importer.py around lines 600 to 615, the test unpacks
the tuple returned by importer.install(result) into (success, message) but never
uses message; change the unpack to (success, _message) or (success, _) to mark
the variable as intentionally unused (or rename to _message) so linters don't
complain, keeping the rest of the test the same.

Comment on lines +714 to +725
def test_verbose_mode(self, capsys):
importer = RequirementsImporter(verbose=True, dry_run=True)

with tempfile.NamedTemporaryFile(mode='w', suffix='.txt', delete=False) as f:
f.write("flask\n")
f.flush()

result = importer.parse_file(f.name)
importer.install(result)

os.unlink(f.name)
# Verbose mode should not crash, dry run prevents actual output
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Remove unused capsys fixture or use it for assertion.

The capsys fixture is declared but never used. Either remove it or add assertions on captured output.

-    def test_verbose_mode(self, capsys):
+    def test_verbose_mode(self):
         importer = RequirementsImporter(verbose=True, dry_run=True)

Alternatively, if you want to verify verbose output:

def test_verbose_mode(self, capsys):
    importer = RequirementsImporter(verbose=True, dry_run=True)
    # ... existing code ...
    captured = capsys.readouterr()
    # Add assertions on captured.out if verbose mode prints anything
🧰 Tools
🪛 Ruff (0.14.7)

714-714: Unused method argument: capsys

(ARG002)

🤖 Prompt for AI Agents
In tests/test_requirements_importer.py around lines 714 to 725 the test function
declares the capsys fixture but never uses it; either remove the unused capsys
parameter from the test signature, or if you want to assert verbose output, keep
capsys and after importer.install(result) call capsys.readouterr() and add
assertions against captured.out (or captured.err) to verify expected verbose
messages; update the test accordingly and run tests to ensure no unused-fixture
warnings.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements a multi-language dependency file importer for Cortex, enabling users to import and install packages from requirements files across Python, Node.js, Ruby, Rust, and Go ecosystems. The implementation provides a unified CLI interface with dry-run capabilities, dev dependency support, and comprehensive error handling.

Key Changes

  • Added requirements_importer.py with parsers for 5 package managers (pip, npm, bundler, cargo, go) with ~650 lines of parsing logic and installation methods
  • Implemented import_cli.py providing a CLI interface with options for single-file import, bulk import, detection, and dry-run modes
  • Created 50+ unit tests covering parser functionality, edge cases, and integration scenarios

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 16 comments.

File Description
cortex/requirements_importer.py Core implementation with parsers for each package manager format and installation logic
cortex/import_cli.py CLI command handlers for import subcommand with argument parsing and output formatting
tests/test_requirements_importer.py Comprehensive test suite covering all parsers, edge cases, and integration workflows
README_IMPORT.md User documentation with usage examples, supported formats, and API reference
Comments suppressed due to low confidence (1)

cortex/requirements_importer.py:493

            elif filename == pattern:

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +29 to +35
def test_dependency_str_with_version(self):
dep = Dependency(name="flask", version="2.0.1")
assert str(dep) == "flask==2.0.1"

def test_dependency_str_without_version(self):
dep = Dependency(name="flask")
assert str(dep) == "flask"
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing test coverage for Dependency.__str__() when the version contains operators (e.g., ">=2.0.0", "~=1.0"). The current tests only cover exact versions ("2.0.1") and no version. Since the parsing logic stores versions with operators (as confirmed by test_parse_with_versions line 75-76), the __str__ method should be tested with these cases to catch the bug where it would produce invalid strings like "package==>=2.0.0".

Copilot uses AI. Check for mistakes.

# Handle extras (e.g., package[extra1,extra2])
extras = []
extras_match = re.match(r'^([a-zA-Z0-9_-]+)\[([^\]]+)\](.*)$', line)
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex pattern r'^([a-zA-Z0-9_-]+)\[([^\]]+)\](.*)$' for parsing extras doesn't properly handle package names with dots (e.g., zope.interface[test]). The character class should include dots: r'^([a-zA-Z0-9_.-]+)\[([^\]]+)\](.*)$'. This inconsistency exists because line 129 correctly allows dots in package names, but line 121 doesn't.

Suggested change
extras_match = re.match(r'^([a-zA-Z0-9_-]+)\[([^\]]+)\](.*)$', line)
extras_match = re.match(r'^([a-zA-Z0-9_.-]+)\[([^\]]+)\](.*)$', line)

Copilot uses AI. Check for mistakes.
Comment on lines +696 to +712
def test_gemfile_complex_syntax(self):
with tempfile.NamedTemporaryFile(mode='w', delete=False) as f:
content = """
source 'https://rubygems.org'

gem 'rails', '7.0.0'
gem 'pg', '>= 0.18', '< 2.0'
"""
f.write(content)
f.flush()

result = GemfileParser.parse(f.name)

os.unlink(f.name)

# Should parse both gems
assert len(result.dependencies) == 2
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test test_gemfile_complex_syntax only verifies that 2 gems are parsed (line 712) but doesn't validate that the version constraints are correctly captured. Given the bug in the parser where only one version constraint is captured for gems with multiple constraints (e.g., gem 'pg', '>= 0.18', '< 2.0'), this test should be enhanced to verify the version field contains the expected constraint information.

Copilot uses AI. Check for mistakes.
Comment on lines +432 to +433
# Skip indirect dependencies
if '// indirect' not in line:
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The go.mod parser checks for the exact string '// indirect' in the line to skip indirect dependencies (line 433). However, Go modules may use different comment styles or whitespace variations. Consider using a more robust check, such as checking if 'indirect' is in the comment portion after splitting by '//', or using regex to handle whitespace variations like '//indirect' or '// indirect '.

Suggested change
# Skip indirect dependencies
if '// indirect' not in line:
# Skip indirect dependencies (robustly detect 'indirect' in comment)
comment = ''
if '//' in line:
comment = line.split('//', 1)[1].strip().lower()
if 'indirect' not in comment:

Copilot uses AI. Check for mistakes.

in_require_block = False

with open(path, 'r') as f:
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File operations should specify encoding explicitly to avoid platform-dependent behavior. The open() calls on lines 76, 174, 242, and 400 don't specify an encoding parameter, which means they use the platform default encoding. This could cause issues when parsing files with non-ASCII characters (e.g., package names or comments with unicode characters). Consider adding encoding='utf-8' to all open() calls to ensure consistent behavior across platforms.

Suggested change
with open(path, 'r') as f:
with open(path, 'r', encoding='utf-8') as f:

Copilot uses AI. Check for mistakes.
Comment on lines +581 to +592
def _install_npm(self, deps: List[Dependency]) -> Tuple[bool, str]:
"""Install Node.js packages via npm."""
packages = [f"{dep.name}@{dep.version}" if dep.version else dep.name for dep in deps]

cmd = ['npm', 'install'] + packages
if self.verbose:
print(f"Running: {' '.join(cmd)}")

result = subprocess.run(cmd, capture_output=True, text=True)
if result.returncode == 0:
return True, f"Installed {len(packages)} packages"
return False, result.stderr
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The subprocess calls should include shell=False (which is the default) to prevent shell injection attacks. However, the commands constructed use user-controlled package names directly in the command arguments. While using a list (not shell=True) provides protection against command injection, there's no validation that package names don't contain special characters that could cause issues. Consider adding validation for package names to ensure they match expected patterns.

Copilot uses AI. Check for mistakes.
Comment on lines +566 to +568
packages.append(f"{dep.name}=={dep.version}" if not any(
op in dep.version for op in ['>=', '<=', '>', '<', '~=', '!=']
) else f"{dep.name}{dep.version}")
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] In the _install_pip method, the version handling logic constructs different strings based on whether the version contains operators. However, the condition if not any(op in dep.version for op in ['>=', '<=', '>', '<', '~=', '!=']) results in f"{dep.name}=={dep.version}" when no operators are found, but f"{dep.name}{dep.version}" when operators are present. This means if a version is ">=2.0.0", the resulting string would be "package>=2.0.0" which is correct. However, if a version is stored as just "2.0.1" (extracted from "==2.0.1" on line 140), it would produce "package==2.0.1" which is also correct. The logic seems correct but the code flow on lines 139-143 where exact versions extract just the version number could lead to confusion since the "==" is stripped and re-added later.

Suggested change
packages.append(f"{dep.name}=={dep.version}" if not any(
op in dep.version for op in ['>=', '<=', '>', '<', '~=', '!=']
) else f"{dep.name}{dep.version}")
packages.append(f"{dep.name}{dep.version}")

Copilot uses AI. Check for mistakes.

def __str__(self) -> str:
if self.version:
return f"{self.name}=={self.version}"
Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The __str__ method in the Dependency class always uses == for version formatting (line 43), but the version field can contain operators like >=, ~=, etc. (as seen in parsing logic on lines 142-143). This means calling str(dep) on a dependency with version ">=2.0.0" would produce "package==>=2.0.0" which is invalid. The __str__ method should handle version specifiers that already contain operators.

Suggested change
return f"{self.name}=={self.version}"
# If version starts with an operator, don't prepend '=='
if self.version.startswith(("==", ">=", "<=", "~=", "!=", ">", "<")):
return f"{self.name}{self.version}"
else:
return f"{self.name}=={self.version}"

Copilot uses AI. Check for mistakes.
Comment on lines +18 to +21
PackageManager,
)


Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import of 'PackageManager' is not used.

Suggested change
PackageManager,
)
)

Copilot uses AI. Check for mistakes.
Comment on lines +11 to +12
import pytest

Copy link

Copilot AI Dec 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Import of 'pytest' is not used.

Suggested change
import pytest

Copilot uses AI. Check for mistakes.
@mikejmorgan-ai
Copy link
Member

@dhvll Could you review this PR? Your experience with the package manager wrapper would be helpful here. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Package Import from Requirements Files

2 participants